Asyncify: Instrument indirect calls from functions in add-list or only-list #2913
Conversation
src/passes/Asyncify.cpp
Outdated
| if (walker.hasIndirectCall && | ||
| (canIndirectChangeState || map[func].addedFromList)) { | ||
| walker.canChangeState = true; | ||
| } |
There was a problem hiding this comment.
It looks like this logic could be combined with the return below to be just one return statement. I think this might be simpler than the the current code.
There was a problem hiding this comment.
How are you suggesting they be combined?
I guess 738 could be return true. Or the 4 lines could be return walker.canChangeState || (walker.hasIndirectCall && (canIndirectChangeState || map[func].addedFromList)). But both seem less good to me?
There was a problem hiding this comment.
I was thinking the latter, but I think the former would also be an improvement. I just prefer to have to track less state and control flow. But this is very much a nit, so I leave it to you :)
There was a problem hiding this comment.
Just to explain why I prefer it as it is, combined with the lines above, this overall code has the form
compute result
// explain exception 1
if ..exception1.. set result to false
// explain exception 2
if ..complex exception2.. set result to true
return result
Actually re-reading it now, those two exceptions are in the wrong order anyhow, so this is moot... reordering, and then the condition before the return is very small so I agree it can be merged with the return, doing it that way.
Co-authored-by: Thomas Lively <[email protected]>
…nges (#11438) Add support for the new ASYNCIFY_ADD_LIST, and update existing list names following the updates in Binaryen, so that now we have ASYNCIFY_ADD_LIST to add a function, ASYNCIFY_REMOVE_LIST to remove one (previously this was called ASYNCIFY_BLACKLIST), and ASYNCIFY_ONLY_LIST to set a list of the only functions to instrument and no others (previously this was called ASYNCIFY_WHITELIST). The updated lists also handle indirect calls properly, so that if you use ASYNCIFY_IGNORE_INDIRECT and then add (using either the add-list or the only-list) all the functions that are on the stack when pausing, then things will work (for more, see WebAssembly/binaryen#2913). A new test is added to show this.
When doing manual tuning of calls using asyncify lists, we want it to
be possible to write out all the functions that can be on the stack when
pausing, and for that to work. This did not quite work right with the
ignore-indirectoption: that would ignore all indirect calls all thetime, so that if
foo()callsbar()indirectly, that indirect call wasnot instrumented (we didn't check for a pause around it), even if
both
foo()andbar()were listed. There was no way to make thatwork (except for not ignoring indirect calls at all).
This PR makes the add-list and only-lists fully instrument the functions
mentioned in them: both themselves, and indirect calls from them.
(Note that direct calls need no special handling - we can just add
the direct call target to the add-list or only-list.)
This may add some overhead to existing users, but only in a function
that is instrumented anyhow, and also indirect calls are slow anyhow,
so it's probably fine. And it is simpler to do it this way instead of
adding another list for indirect call handling.